Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix other options are forbidden when --no-source-map #59

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

smnandre
Copy link
Contributor

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this looks good to me technically.

But won't it cause "WTH" problems when users will see those options in the bundle's configuration but they will be silently ignored? I mean, won't it be better to let the Sass binary throw an error so you could adjust bundle's config instead?

I laterally mean doing something like this in config:

symfonycasts_sass:
    sass_options:
        source_map: false
        embed_sources: false
        embed_source_map: false

This config should solve the problem, right?

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Jan 17, 2024

Lol, ok, now I see myself. The problem is that you get that --embed-sources isn't allowed with --no-source-map error no matter if you pass --embed-sources or --no-embed-sources... which is weird, I would suppose --no-embed-sources works because the error message clearly continues saying about --embed-sources only 🤦‍♂️

So, IMO their (Sass) check isn't ideal as it should allow --no-embed-sources with --no-source-map... because why not? And if it was allowed - we would not need this fix at all.

Anyway, how about mentioning this in the docs then? :) probably a comment over the source_map option is enough. I suppose we should say something like:

If source_map is false - embed_sources and embed_source_map options are ignored

@smnandre
Copy link
Contributor Author

smnandre commented Jan 17, 2024

I wanted to suggest a change on their side... but it's implemented in that way since .... a long time.

This is a quick fix.

What i'd like to do next is use the "enable" symfony config option.

So you'd have something like this

symfonycasts_sass:
    sass_options:
    
        # Nothing 
        source-map: false

        # Source map ON  (no embed source)
        source-map: true

        # Source map ON and embed source
        source-map: 
            embed-sources: true
            
        # Source map OFF (and nothing else)
        source-map: 
            enabled: false
            embed-sources: false            
            
        # etc etc    

But really i'd release this fix now as it forbid people to compile their sass file in PROD env :(

@bocharsky-bw
Copy link
Member

I wanted to suggest a change on their side... but it's implemented in that way since .... a long time.

Yeah, not a big deal to just work it around I think 👍

Thank you for this quick fix!

What i'd like to do next is use the "enable" symfony config option.

That sounds like an interesting idea that would make things clearer... but it would also lead to a BC break in the configuration, though we could probably overcome it with some code complications.

@bocharsky-bw bocharsky-bw merged commit ab60d02 into SymfonyCasts:main Jan 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to disable source maps
2 participants